Add SDL_RenderDebugTextF & SDL_RenderDebugTextV #11602
Conversation
|
@icculus, is this something we want? |
|
Mmmm...I don't think we should add this. It's trivial enough to use SDL_snprintf() directly. |
|
Potentially, but I feel using a single function over two is cleaner and less verbose, especially if you're repeatedly formatting and rendering debug text to display multiple lines of data. If you feel like it isn't worth the extra functions though, that's understandable. |
|
I think the F variant is worth adding, but I don't think this is something people should generally be wrapping other format print functions around, so let's drop the V variant. @icculus, technically it's an ABI break, but can we just switch the existing function to take format params instead of a single text pointer? 99.99% of uses will not contain '%', and should be safe as-is. |
|
I was thinking it would be better if the one function just accepted format args, had we thought of that before shipping a prerelease. We can just add a second function, and reduce it in SDL4. :) |
|
Okay, we did some experimentation, and we think that just changing the existing function to take a format string is binary compatible across a bunch of architectures, and the risk overall of changing this is small at this point, as it's only in one prerelease, so we're going to take a shot at it. This is totally pushing the boundaries on ABI stability, but we think it's the correct technical decision. I'm going to merge this PR so you get credit for the work, then make the changes on top of that. |
This should make it easier to quickly put important numbers and such on the screen without having to format them into a string manually.
Co-authored-by: Anonymous Maarten <[email protected]>
icculus
force-pushed
the
formatted-debug-text
branch
from
329f619 to
567d87c
Compare
|
Okay, this is rebased and updated to just have the original function offer printf-style formatting. Let the builders and @slouken check it, then go ahead and merge! |
| @@ -2519,6 +2521,13 @@ extern SDL_DECLSPEC bool SDLCALL SDL_GetRenderVSync(SDL_Renderer *renderer, int | |||
| * | |||
| * The text is drawn in the color specified by SDL_SetRenderDrawColor(). | |||
| * | |||
| * **WARNING**: Note that in SDL 3.1.6, this function accepted a single string | |||
There was a problem hiding this comment.
I would remove this warning. We can put it in the patch notes, but we don't need to carry it forward.
| if (result >= 0 && (size_t)result >= sizeof(buf)) { \ | ||
| str = NULL; \ | ||
| va_start(ap, fmt); \ | ||
| result = SDL_vasprintf(&str, fmt, ap); \ |
There was a problem hiding this comment.
Do you need the jump table version of this?
|
I'm looking at these changes, wondering if maybe we should just have a separate function. Having to add "%s" to all the existing call sites seems clunky. |
|
This brings me back to "they can just call snprintf and hand us a string" then. :/ |
Fair enough. :) |
|
Just saw this, ah well, too bad I suppose. Maybe next time :) |
|
Your patch was well-constructed, to be clear, we just decided against adding the API here. |
|
It also doesn’t mean we won’t add it later, we are just trying to keep things lean and focused for release. |
Description
SDL_RenderDebugTextis a useful function to quickly put debug text on the screen, but provides no easy way to display other data useful for debugging such as numbers. In an attempt to rectify that, I've addedSDL_RenderDebugTextFas well asSDL_RenderDebugTextV. These functions allow you to pass in a format string as well as the formatting arguments (either variadicly or as ava_listrespectively) and will run them throughSDL_vsnprintfbefore itself callingSDL_RenderDebugTextto do the actual rendering. This should hopefully make it easier to quickly put useful data such as framerate, particle count, or whatever else is needed onto the screen without having to format them into a string manually.I'm not too sure if I've done everything correctly here (mainly I wasn't sure what to put in the
\sincedocumentation tags, I just used 3.1.7 as that's what was in CMakeLists.txt), but hopefully it's not too bad :)Existing Issue(s)
No issue.